Use concrete error types for ipld/* and shared by dignifiedquire · Pull Request #463 · filecoin-project/ref-fvm

dignifiedquire · 2022-04-06T14:27:08Z

This is the first step to be able to remove the usage of downcasting in actors.
It removes the usage of anyhow in lower level parts.

vmx · 2022-04-06T14:34:26Z

fvm/src/blockstore/buffered.rs

+    #[error("cbor input was not canonical (lval 24 with value < 24)")]
+    HeaderLval24,
+    #[error("cbor input was not canonical (lval 25 with value <= MaxUint8)")]
+    HeaderLval25,
+    #[error("cbor input was not canonical (lval 26 with value <= MaxUint16)")]
+    HeaderLval26,
+    #[error("cbor input was not canonical (lval 27 with value <= MaxUint32)")]
+    HeaderLval27,


Without knowing what they are really about, I'd collapse them into HeaderLval(u8).

Probably, I am deferring to figuring out the best thing to @raulk or @Stebalien (not sure who wrote this code)

Stebalien · 2022-04-06T16:05:56Z

We switched to anyhow because:

These errors aren't useful in practice. We can't actually do anything other than to give the user a nice error message telling them what's wrong.
Converting between and handling errors became a real pain.

dignifiedquire · 2022-04-06T16:07:14Z

@Stebalien well I would argue they are very useful, because you don't need to downcast anymore, you can actually do type level checks in actors with this

Stebalien · 2022-04-06T16:23:17Z

@Stebalien well I would argue they are very useful, because you don't need to downcast anymore, you can actually do type level checks in actors with this

Sorry, I should have been specific. I meant having many detailed errors rather than just a simple "here's what went wrong" error message.

On the other hand, they don't doesn't really hurt, I guess.

But please propagate before merging here. I agree with you in theory, but enumerated errors became a real pain in practice.

Anyhow allowed us to just combine arbitrary errors. This makes propagating errors really easy.
Error enums means you need to either:
- Define a different enum for every case.
- Define a single enum that includes every sub-error in your crate, which is pretty useless (no way to tell which errors can actually happen in any given case).
Making the blockstore generic over the error tainted everything with additional generics.

On the other hand, we started with an unholy mix of enumerated errors, string errors, and anyhow. You may have better luck now.

anorth

Looks great to me, but please wait for someone else's review too.

anorth · 2022-04-06T21:48:33Z

ipld/amt/Cargo.toml

 once_cell = "1.5"
 ahash = { version = "0.7", optional = true }
 itertools = "0.10"
-anyhow = "1.0.51"


anorth · 2022-04-06T21:52:13Z

ipld/amt/src/amt.rs

            let found = self.delete(i)?.is_none();
            if strict && found {
-                return Err(anyhow!("no such index {} in Amt for batch delete", i).into());
+                return Err(Error::BatchDelteNotFound(i));


Typo in BatchDelteNotFound

anorth · 2022-04-06T21:54:24Z

ipld/amt/src/error.rs

-        Self::Dynamic(anyhow::anyhow!(e))
-    }
+#[derive(Debug, Error)]
+pub enum EitherError<U, E> {


Please add some comments explaining this. I get that it's unioning two types of errors, but don't understand why or how it should be used. What's the distinction between the two types of errors? If raising, how do I chose which one? If handling, what can I infer from the different types?

added some docs

anorth · 2022-04-06T21:54:38Z

ipld/amt/src/error.rs

+    #[error("user: {0}")]
+    User(U),
+    #[error("hamt: {0}")]
+    Hamt(#[from] Error<E>),


This is AMT not HAMT

that’s what I get for copy pasting 😬

dignifiedquire · 2022-04-06T22:56:31Z

once the exit code PR lands on builtin-actors, I will make a matchinh PR, to make sure things work as intended based on this.

Stebalien · 2022-04-08T08:40:15Z

fvm/src/blockstore/buffered.rs

+    #[error("string in cbor input too long")]
+    StringTooLong,
+    #[error("Invalid link ({0}) in flushing buffered store")]
+    InvalidLink(Cid),


MissingBlock?

Stebalien · 2022-04-08T08:46:00Z

fvm/src/blockstore/buffered.rs

 }

+#[derive(thiserror::Error, Debug)]
+pub enum FlushError {


I'd reduce this to:

MissingBlock (can't find a block)

IPLD (encoding error)

(how can we even have an IO error that's not a blockstore error?)

If we're super-specific, we'll have to break this error type every time we change some detail (switch to libipld's link enumeration function, support new IPLD codecs, etc.).

Stebalien · 2022-04-08T08:48:28Z

ipld/blockstore/src/lib.rs

 ///
 /// The cgo blockstore adapter implements this trait.
 pub trait Blockstore {
+    type Error: std::error::Error + std::fmt::Debug + Send + Sync + 'static;


This is what we started with, but it was a huge pain.

This is incompatible with anyhow, because anyhow doesn't implement std::error::Error.

Repeating this kind of constraint wherever we want to be generic is really annoying.

What if we just don't constrain this? Really, only applications care about enforcing constraints on this type.

That is true, the main reason I constrained it is because anyhow::Context requires this, and so it gets painful there, as I can't easily define a where clause on Machine::Blockstore::Error without having to write it in a LOT of places.

Yeah, I guess that's fine. We just need to make sure we have patches for the actors before we merge.

Stebalien · 2022-04-08T08:50:17Z

ipld/hamt/src/pointer.rs

    /// Internal method to cleanup children, to ensure consistent tree representation
    /// after deletes.
-    pub(crate) fn clean(&mut self) -> Result<(), Error> {
+    pub(crate) fn clean<BS: Blockstore>(&mut self) -> Result<(), Error<BS::Error>> {


Can you use ! or convert::Infallable as the blockstore error?

If I do that, the error propagation stops working when I call it. If you have an idea how to work around it, I am all ears

maciejwitowski · 2022-10-10T14:46:38Z

@Stebalien This is assigned to M2.1 now. Is it correct?

Stebalien · 2022-10-13T03:20:42Z

It probably won't land in M2.1. I'd like it to, but I expect we'll handle this in M2.2.

dignifiedquire mentioned this pull request Apr 6, 2022

Use new ExitCode type and values, change ActorError to make bad exit codes less likely filecoin-project/builtin-actors#191

Merged

1 task

dignifiedquire requested review from Stebalien and anorth April 6, 2022 14:28

vmx reviewed Apr 6, 2022

View reviewed changes

anorth approved these changes Apr 6, 2022

View reviewed changes

Stebalien reviewed Apr 8, 2022

View reviewed changes

dignifiedquire force-pushed the errors-round1 branch from d738cb2 to cd07afe Compare April 11, 2022 16:52

dignifiedquire mentioned this pull request Apr 12, 2022

Remove downcasting and use explicit errors filecoin-project/builtin-actors#214

Draft

1 task

dignifiedquire force-pushed the errors-round1 branch 2 times, most recently from ad2998a to c1f1026 Compare April 19, 2022 11:32

dignifiedquire force-pushed the errors-round1 branch from c1f1026 to 8a68e6a Compare April 22, 2022 14:03

dignifiedquire added 7 commits April 28, 2022 19:59

start patching through errors in ipld

d46362a

convert ipld/hamt

bcc4fbe

convert ipld/amt

6a0a750

convert shared

4480da3

convert fvm and update type bounds accordingly

8b9dc8f

apply cr

fd9f106

additions based on integration into builtin-actors

4b20a04

dignifiedquire force-pushed the errors-round1 branch from 8a68e6a to 4b20a04 Compare April 28, 2022 17:59

raulk added this to the Phase 2 milestone May 5, 2022

raulk added the Hint: Parked label May 5, 2022

Stebalien self-requested a review June 28, 2022 05:23

alexytsu mentioned this pull request Jul 19, 2022

Evaluate dropping the dependency on anyhow filecoin-project/actors-utils#15

Closed

maciejwitowski mentioned this pull request Oct 10, 2022

FEVM Development Checklist #936

Closed

85 tasks

Stebalien modified the milestones: M2.1, M2.2 Oct 13, 2022

rjan90 marked this pull request as draft August 20, 2024 21:12

Conversation

dignifiedquire commented Apr 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Stebalien commented Apr 6, 2022

Uh oh!

dignifiedquire commented Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Stebalien commented Apr 6, 2022

Uh oh!

anorth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dignifiedquire commented Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maciejwitowski commented Oct 10, 2022

Uh oh!

Stebalien commented Oct 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dignifiedquire commented Apr 6, 2022 •

edited

Loading

dignifiedquire commented Apr 6, 2022 •

edited

Loading